Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: update test_2799 for Numba 0.59.0 #2998

Merged
merged 4 commits into from
Feb 2, 2024

Conversation

jpivarski
Copy link
Member

Numba 0.59.0 is now available (on PyPI), so test_2799 was failing because its first line was

raise NotImplementedError

I think you were waiting on information about how Numba would be dealing with this issue when 0.59.0 comes out. Your expectation that flattened.dtype would be int8 doesn't seem to be right. Experimentally, it returns int64. Even for pure NumPy (no Awkward Array):

>>> import numba
>>> import numpy as np
>>> @numba.vectorize(nopython=True)
... def add(x, y):
...     return x + y
... 
>>> array = np.array([1, 2, 3, 4], np.int8)
>>> result = add(array, np.int16(np.iinfo(np.int8).max + 1))
>>> result.dtype
dtype('int64')

I've updated the test with this correction (what I believe to be a correction).

#2996 and #2997 both need it to be merged, and they need to be merged to release Awkward 2.6.0. (In fact, any subsequent runs of the CI tests will need this, since they get Numba through PyPI and that's 0.59.0 now.)

Two other-library updates came out on the same day, both of which complicate our tests! (pytest 8.0.0 came out, which breaks pytest-asyncio, and Numba 0.59.0 came out.)

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0eb487a) 81.89% compared to head (a7b943d) 81.89%.
Report is 3 commits behind head on main.

Additional details and impacted files

@jpivarski
Copy link
Member Author

This is just the sort of thing that comes out of nowhere and stops a release:

  C:\hostedtoolcache\windows\Python\3.9.13\x86\python.exe C:\Users\runneradmin\AppData\Local\Temp\pip-install-921_u8fz\llvmlite_bdff54372838458392dbf1151245b0b5\ffi\build.py
  CMake Warning (dev) at CMakeLists.txt:3 (project):
    cmake_minimum_required() should be called prior to this top-level project()
    call.  Please see the cmake-commands(7) manual for usage documentation of
    both commands.
  This warning is for project developers.  Use -Wno-dev to suppress it.

  CMake Error at CMakeLists.txt:3 (project):
    Generator

      Visual Studio 16 2019

    could not find any instance of Visual Studio.

Is it saying that we need just one more thing in CMakeLists.txt? If so, why did this never come up before, and why only on one version of one build?

Why, o why?

@jpivarski
Copy link
Member Author

In fact, cmake_minimum_required() is called prior to the top-level project()!

# BSD 3-Clause License; see https://github.com/scikit-hep/awkward-1.0/blob/main/LICENSE
cmake_minimum_required(VERSION 3.15...3.26)
# Project must be near the top
project(
${SKBUILD_PROJECT_NAME}
LANGUAGES CXX
VERSION ${SKBUILD_PROJECT_VERSION})

@agoose77
Copy link
Collaborator

agoose77 commented Feb 2, 2024

@jpivarski no, that's a symptom that the build environment on Windows is not working -- we need to discover the C++ compilers to be able to proceed.

The OS is windows-latest and the arch is 32-bit -- my guess is that the windows image has updated and doesn't include VS compilers for 32-bit.

I'm spitballin' as they say, but I don't think it's necessarily a problem. We can check to see whether the windows image has changed, for example.

@jpivarski
Copy link
Member Author

windows-latest is windows-2022, which doesn't sound like a recent update. Maybe that page is old. Perhaps we can explicitly set the Windows runner to windows-2019 or windows-2022 (on the assumption that the runner-images page is old).

@jpivarski
Copy link
Member Author

I'll try windows-2019.

@agoose77
Copy link
Collaborator

agoose77 commented Feb 2, 2024

OK, it seems unlikely that the image has changed. I note that this fails later when trying to build llvmlite. I suspect this is because Numba no longer releases wheels for this platform - I know that they've just released a new version. But, I think that would be a red herring - we should see the same error in windows-latest. Let me see if cmake has changed.

@agoose77
Copy link
Collaborator

agoose77 commented Feb 2, 2024

This is ultimately being caused by pip trying to build an sdist of llvmlite because PyPI doesn't have a 32-bit wheel. I am content not to get to the bottom of why this happens, but I suspect the traceback is caused by llvmlite's internal detection logic (https://github.com/numba/llvmlite/blob/f22420ad768a31dd15ae45d551bb4e73907a27fa/ffi/build.py#L56). This follows from the fact that an awkward-cpp build succeeds in the same job (although it might use a different CMake from PyPI, that's irrelevant).

Off-hand I can't remember @jpivarski when we plan to drop 32-bit windows. It won't be hard to keep it running for now; we just need disable numba on win (32bit)

@jpivarski
Copy link
Member Author

That's a good idea—dropping Numba on the 32-bit Windows test. I'll do that.

The Windows 32-bit test was historically very valuable in helping us eliminate cases in which we wrongly assumed that intp is int64 in our C++ code. There's no reason to test that with Numba, but it's good to keep testing it as long as we might edit the C++ code and Windows 32-bit is still a common enough platform to consider targeting.

You've also answered the "why now?" question: because Numba released 0.59.0 so recently.

@jpivarski jpivarski merged commit 7f2f219 into main Feb 2, 2024
39 checks passed
@jpivarski jpivarski deleted the jpivarski/update-test_2799-for-Numba-0.59.0 branch February 2, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants